Improve inference by not considering thisless functions to be context-sensitive #62243
+4,698
−56
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
typescript-bot
added
the
For Uncommitted Bug
There was a problem hiding this comment.
Pull Request Overview
This PR improves TypeScript's type inference by making functions that don't reference this not context-sensitive. Previously, all object literal methods were considered context-sensitive, causing poor inference when type parameters depended on inferring from these methods first. The change helps TypeScript better infer types in common patterns involving object literals with functions.
Key Changes:
- Modified
hasContextSensitiveParametersto check for actualthisusage rather than assuming all function-like declarations are context-sensitive - Updated binder to track
thiskeyword usage withNodeFlags.ContainsThisflag - Extended context-sensitive checking to include yield expressions in generators
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/compiler/binder.ts | Adds tracking of this keyword usage and sets ContainsThis flag on functions |
| src/compiler/checker.ts | Updates context-sensitive checking to include yield expressions and generators |
| src/compiler/utilities.ts | Modifies hasContextSensitiveParameters and forEachYieldExpression to support new logic |
| tests/cases/compiler/*.ts | New test cases demonstrating improved inference for thisless functions |
| tests/baselines/reference/. | Updated baselines showing improved type inference results |
| @@ -2889,14 +2894,13 @@ export function forEachYieldExpression(body: Block, visitor: (expr: YieldExpress | |||
| if (node.name && node.name.kind === SyntaxKind.ComputedPropertyName) { | |||
| // Note that we will not include methods/accessors of a class because they would require | |||
| // first descending into the class. This is by design. | |||
| traverse(node.name.expression); | |||
| return; | |||
| return traverse(node.name.expression);; | |||
| } | |||
There was a problem hiding this comment.
There's a double semicolon at the end of this line. Remove the extra semicolon.
Copilot uses AI. Check for mistakes.
| // Reset all reachability check related flags on node (for incremental scenarios) | ||
| node.flags &= ~NodeFlags.ReachabilityAndEmitFlags; | ||
| // Reset flags (for incremental scenarios) | ||
| node.flags &= ~(NodeFlags.ReachabilityAndEmitFlags | NodeFlags.ContainsThis); |
There was a problem hiding this comment.
[nitpick] The comment above this line mentions 'Reset all reachability check related flags' but now it also resets ContainsThis. Consider updating the comment to reflect that other flags are also being reset.
Suggested change
| node.flags &= ~(NodeFlags.ReachabilityAndEmitFlags | NodeFlags.ContainsThis); | |
| // Reset reachability, emit, and ContainsThis flags (for incremental scenarios) | |
| node.flags &= ~(NodeFlags.ReachabilityAndEmitFlags | NodeFlags.ContainsThis); |
Copilot uses AI. Check for mistakes.
src/compiler/binder.ts
Outdated
| if (!(currentFlow.flags & FlowFlags.Unreachable) && containerFlags & ContainerFlags.IsFunctionLike && nodeIsPresent((node as FunctionLikeDeclaration | ClassStaticBlockDeclaration).body)) { | ||
| node.flags |= NodeFlags.HasImplicitReturn; | ||
| if (hasExplicitReturn) node.flags |= NodeFlags.HasExplicitReturn; | ||
| (node as FunctionLikeDeclaration | ClassStaticBlockDeclaration).endFlowNode = currentFlow; | ||
| } | ||
| if (seenThisKeyword) { | ||
| node.flags |= NodeFlags.ContainsThis; |
There was a problem hiding this comment.
There's trailing whitespace at the end of this line. Remove the extra spaces.
Suggested change
| node.flags |= NodeFlags.ContainsThis; | |
| node.flags |= NodeFlags.ContainsThis; |
Copilot uses AI. Check for mistakes.
Andarist
commented
Aug 9, 2025
| @@ -3941,28 +3951,27 @@ export function getContainerFlags(node: Node): ContainerFlags { | |||
| // falls through | |||
| case SyntaxKind.Constructor: | |||
| case SyntaxKind.FunctionDeclaration: | |||
| case SyntaxKind.ClassStaticBlockDeclaration: | |||
| return ContainerFlags.IsContainer | ContainerFlags.IsControlFlowContainer | ContainerFlags.HasLocals | ContainerFlags.IsFunctionLike; | |||
| case SyntaxKind.MethodSignature: | |||
| case SyntaxKind.CallSignature: | |||
| case SyntaxKind.JSDocSignature: | |||
| case SyntaxKind.JSDocFunctionType: | |||
| case SyntaxKind.FunctionType: | |||
| case SyntaxKind.ConstructSignature: | |||
| case SyntaxKind.ConstructorType: | |||
There was a problem hiding this comment.
Type-level AST nodes had ContainerFlags.IsControlFlowContainer here. This was messing up some of the changes I made since it was interfering with the implemented seenThisKeyword tracking. I don't see why those would be considered control flow containers and there are no tests proving it was needed.
Other changes in this function are basically of the same kind - I just removed ContainerFlags.IsControlFlowContainer from the type-level nodes.
Andarist
commented
Aug 9, 2025
| @@ -21146,7 +21147,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| } | |||
|
|
|||
| function isContextSensitiveFunctionLikeDeclaration(node: FunctionLikeDeclaration): boolean { | |||
| return hasContextSensitiveParameters(node) || hasContextSensitiveReturnExpression(node); | |||
| return hasContextSensitiveParameters(node) || hasContextSensitiveReturnExpression(node) || !!(getFunctionFlags(node) & FunctionFlags.Generator && node.body && forEachYieldExpression(node.body as Block, isContextSensitive)); | |||
There was a problem hiding this comment.
Previously generators were always context-sensitive as they can't even be arrow functions. At times, they are truly context-sensitive in cases like:
declare function test(
gen: () => Generator<(arg: number) => string, void, void>,
): void;
test(function* () {
yield (arg) => String(arg);
});So I had to add this extra forEachYieldExpression to cover for this
Andarist
commented
Aug 9, 2025
| /** @internal */ | ||
| export function forEachYieldExpression(body: Block, visitor: (expr: YieldExpression) => void): void { | ||
| export function forEachYieldExpression<T>(body: Block, visitor: (expr: YieldExpression) => T): T | undefined { |
There was a problem hiding this comment.
this basically changes this function to work in the same way as forEachReturnStatement defined above
Andarist
commented
Aug 9, 2025
| @@ -68,8 +68,8 @@ declare var connect: Connect; | |||
| const myStoreConnect: Connect = function( | |||
| >myStoreConnect : Connect | |||
| > : ^^^^^^^ | |||
| >function( mapStateToProps?: any, mapDispatchToProps?: any, mergeProps?: any, options: unknown = {},) { return connect( mapStateToProps, mapDispatchToProps, mergeProps, options, );} : <TStateProps, TOwnProps>(mapStateToProps?: any, mapDispatchToProps?: any, mergeProps?: any, options?: unknown) => InferableComponentEnhancerWithProps<TStateProps, Omit<P, Extract<keyof TStateProps, keyof P>> & TOwnProps> | |||
There was a problem hiding this comment.
this just changes the inferred type to match the type inferred from the equivalent arrow function with type parameters, it's purely a result of making a thisless function context-insensitive
Andarist
commented
Aug 9, 2025
| @@ -117,8 +117,8 @@ export const Nothing2: Strategy<State> = strategy("Nothing", function*(state: St | |||
| export const Nothing3: Strategy<State> = strategy("Nothing", function* (state: State) { | |||
| >Nothing3 : Strategy<State> | |||
| > : ^^^^^^^^^^^^^^^ | |||
| >strategy("Nothing", function* (state: State) { yield ; return state; // `return`/`TReturn` isn't supported by `strategy`, so this should error.}) : (a: State) => IterableIterator<State, void> | |||
| > : ^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | |||
| >strategy("Nothing", function* (state: State) { yield ; return state; // `return`/`TReturn` isn't supported by `strategy`, so this should error.}) : (a: any) => IterableIterator<any, void> | |||
There was a problem hiding this comment.
similarly here, this is a result of inferSignatureInstantiationForOverloadFailure no longer skipping the generator function on the basis it's context-sensitive (inferSignatureInstantiationForOverloadFailure uses CheckMode.SkipContextSensitive)
Andarist
commented
Aug 9, 2025
| @@ -271,16 +271,16 @@ let impl: I = { | |||
| }, | |||
| } | |||
| impl.explicitVoid1 = function () { return 12; }; | |||
| >impl.explicitVoid1 = function () { return 12; } : (this: void) => number | |||
There was a problem hiding this comment.
those this parameters were not used by the assigned implementation - they were just auto-assigned to it based on the this parameter in the contextual signature
|
@typescript-bot test it |
|
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
Hey @jakebailey, the results of running the DT tests are ready. There were interesting changes: Branch only errors:Package: jqrangeslider |
|
@jakebailey Here are the results of running the user tests with tsc comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Something interesting changed - please have a look. Details
|
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@jakebailey Here are the results of running the top 400 repos with tsc comparing Something interesting changed - please have a look. Details
|
|
…dates-based returns `any`/`unknown`
|
@typescript-bot test it |
|
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
Hey @jakebailey, the results of running the DT tests are ready. There were interesting changes: Branch only errors:Package: jqrangeslider |
|
@jakebailey Here are the results of running the user tests with tsc comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Something interesting changed - please have a look. Details
|
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
implements @RyanCavanaugh's suggestion from #47599 :
fixes #62204
fixes #60986
fixes #58630
fixes #57572
fixes #56067
fixes #55489
fixes #55124
fixes #53924
fixes #50258